-
Notifications
You must be signed in to change notification settings - Fork 5
fix: separate pkg manager completion handling from the core API #31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
commit: |
| completion.addCommand('test', 'Run tests', [], noopCompletion); | ||
| completion.addCommand('build', 'Build project', [], noopCompletion); | ||
| export function setupPnpmCompletions(completion: PackageManagerCompletion) { | ||
| completion.command('add', 'Install a package'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i do not see any completion for all of these commands, we should provide for all of them before release. that's what we agreed on as far as i remember.
| { value: 'tests/**/*.ts', description: 'Test files' }, | ||
| ]; | ||
| }; | ||
| if (command.value === 'lint') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the whole commander api needs a new look.
| } | ||
| } | ||
|
|
||
| async function getCliCompletions( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this function and checkCliHasCompletions has so much redundancy and i feel they're heavily similar.
| * This extends RootCommand and adds package manager-specific logic. | ||
| * It acts as a layer on top of the core tab library. | ||
| */ | ||
| export class PackageManagerCompletion extends RootCommand { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i do not think we need this class, it can be done through some imperative functions and calls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's remove it and revisit the implementation. it's a bit hard to understand this implementation for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and we need tests for the package manager completions.
now, pkg managers completions are done through a wrapper.